-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tests for clap::helpers::Host #50
Conversation
7fea6ba
to
aa26d48
Compare
include/clap/helpers/host.hxx
Outdated
@@ -428,15 +428,15 @@ namespace clap { namespace helpers { | |||
/////////////// | |||
template <MisbehaviourHandler h, CheckingLevel l> | |||
Host<h, l> &Host<h, l>::from(const clap_host *host) noexcept { | |||
if constexpr (l >= CheckingLevel::Minimal) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ouch, maybe it is time to require C++17? ;-)
It is 2024 so it is already 7 years old.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I would be fine with that
Maybe @baconpaul can tell? My original idea was to use clap::helpers::Host
for clap-wrapper as well. So it should meet the requirements there, too. I think somewhere I read that they wrote something like "c++17 min" but just want to ensure that it would be OK.
The CMakeLists.txt defaulted to 11 before we started upgrading it recently and still does I think. So I guess we would have to change it there as well (?)
(edit: I justed looked it up: at least the clap-helpers-tests
default to 11 so that it looks like it is the minimum.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok just removed the CLAP_IF_CONSTEXPR
We can solve the rest (c++11 etc) later
|
||
#include <catch2/catch_all.hpp> | ||
|
||
template class clap::helpers::Host<clap::helpers::MisbehaviourHandler::Terminate, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we want to use a macros to expand all the combinations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, maybe in a later PR?
Thanks the PR looks good except the As of today C++17 is well supported on Windows, macOS and Linux, maybe we just upgrade the requirement? |
No description provided.